fixtures: use a better data structure for storing FixtureDefs - #14984
Conversation
Let's use consistent terminology.
This is an internal function, so it's not a official guarantee, but let's explicitly document the behavior internally at least.
psf-chronographer
Bot
added
the
bot:chronographer:provided
…ide the class It's easier to handle this way.
690d7d4 to
d938a56
Compare
| # Legacy fallback, for plugins still using the deprecated nodeid-based | ||
| # API without a node reference. | ||
| # Part of FIXTURE_NODEID_DEPRECATED deprecation. | ||
| self._arg2nodeid2fixturedefs: Final[ |
There was a problem hiding this comment.
Instead of a separate datastructure, is it possible for us to get the nodeid from the session collection tree? Or perhaps the node might not exist at this point yet?
There was a problem hiding this comment.
If you mean that the single data structure will work with nodeids, it's possible and I think it will work, but then we're back to working with nodeids and I want to avoid that.
If you mean going the other way, i.e. getting a Node given a nodeid, we don't have a way to do it other than searching through the collection tree, which will be slow.
I went with a similar solution to what we did for autouse names (_nodeid_autousenames). In pytest 10.1 we just delete all of that...
There was a problem hiding this comment.
Yeah I meant the 2nd one (only using Node) -- but I agree with the solution then, as we will eventually just delete the 2nd datastructure once we drop that support. 👍
Previously, FixtureManager stored the registered FixtureDefs in
`_arg2fixturedefs` which is
<fixutre name> -> [FixtureDef]
where the FixtureDefs are ordered by visibility.
There are two inefficiencies with this:
1. When registering a fixture, we need to find the appropriate index to
insert in the list. This is done with slow quadratic
`is_visibility_more_specific` checks.
Before 7186cd4, FixtureDefs were
always appended, relying on the collection order, so there was no
quadratic issue. But then we added `pytest.register_fixture` which is
not guaranteed to be called in collection order.
This is pytest-dev#14942, introduced in v9.1.0.
2. When looking up FixtureDefs for a node, the entire list needed to be
filtered for visibility to the node (`_matchfactories`). With many
fixtures registered with the same name (even if completely
unrelated), this can be slow.
This is an old issue.
Change the way we store the FixtureDefs to `_arg2node2fixturedefs`,
which is
<fixture name> -> (Node -> [FixtureDef])
i.e. instead of storing the FixtureDefs for a name in a single big list,
store them by the Node under which they are registered.
This fixes (1) since now just need to append to
`arg2fixture2nodes[name][node]`.
Fixes (2) since no longer need to filter a big list (scaling with number
of fixtures registered with same name). Instead need to look up the
FixtureDefs registered for the node and its ancestors (scales with
height of the collection tree, which should be OK).
Fix pytest-dev#14942
d938a56 to
aeb5b3e
Compare
|
@nicoddemus I fixed the changelog, and also added a couple of tests for coverage of the nodeid paths. The remaining coverage miss is preexisting, which look to me like a leftover from a reverted "invocation-scoped fixtures" feature which was before my time (code added in 44ecf2a). |
|
Cool, LGTM! |
|
@nicoddemus missclick? |
|
Ouch yes! Probably hit "Enter" at the wrong moment, apologies. |
Moving a fixture into a conftest one directory over leaves the name registered but out of scope, and the error said only that it was "not found" plus a list of the fixtures that *were* available. Neither half mentions the definition sitting in the tree, so the reader goes looking for a name that is already in front of them. List those definitions. The fixturedefs registered under a name are now a dict keyed by node (pytest-dev#14984), so enumerating them regardless of visibility is a lookup rather than a scan -- which is what makes this cheap enough to do on the error path. Past five distinct locations the paths stop being a hint and start being a wall, so collapse the tail into a count. The hint only sees conftests that this run actually loaded. Collecting a single file does not load its siblings, so the case that prompted the issue reports nothing when run that narrowly; a whole-suite run, which is how the confusion arises in the first place, has them all. Fixes pytest-dev#10151. Co-Authored-By: Claude Opus 5 (1M context) via Claude Code <noreply@anthropic.com>
3 participants
This PR fixes #14942.
The commits
The 1st and 2nd commits are related internal documentation fixes.
The 3rd commit is a little refactor to encapsulate access to
FixtureManager._arg2fixturedefto make the next commit clearer.The 4th commit is main change, I reproduce the commit message below.
The 5th commit adds backward compat for string nodeids in
register_fixture. I split it from the previous commit to make it easier to review the main change without the ugly compat. I will squash it before merging.Description
Previously, FixtureManager stored the registered FixtureDefs in
_arg2fixturedefswhich iswhere the FixtureDefs are ordered by visibility.
There are two inefficiencies with this:
When registering a fixture, we need to find the appropriate index to insert in the list. This is done with slow quadratic
is_visibility_more_specificchecks.Before 7186cd4, FixtureDefs were always appended, relying on the collection order, so there was no quadratic issue. But then we added
pytest.register_fixturewhich is not guaranteed to be called in collection order.This is Test collection is 10x slower on 9.1.1 compared to 9.0.3 #14942, introduced in v9.1.0.
When looking up FixtureDefs for a node, the entire list needed to be filtered for visibility to the node (
_matchfactories). With many fixtures registered with the same name (even if completely unrelated), this can be slow.This is an old issue.
Change the way we store the FixtureDefs to
_arg2node2fixturedefs, which isi.e. instead of storing the FixtureDefs for a name in a single big list, store them by the Node under which they are registered.
This fixes (1) since now just need to append to
arg2fixture2nodes[name][node].Fixes (2) since no longer need to filter a big list (scaling with number of fixtures registered with same name). Instead need to look up the FixtureDefs registered for the node and its ancestors (scales with height of the collection tree, which should be OK).
Performance
Performance numbers with reproducer from #14942 (comment) (
NCLASSES=10000 time pytest reproducer.py):It's still kinda slow, but a profile shows that's for other preexisting reasons. The improvement over main is due to fixing problem (1), and over before the regression due to fixing problem (2).
Backward compat
This breaks plugins which directly access
FixtureManager._arg2fixturedefs(double private 😀). From my local corpus (678 plugins), I see it done in these plugins:I think this is acceptable.